Skip to content

Fail closed when gh is absent during attestation#21

Merged
TeoSlayer merged 1 commit into
mainfrom
fix/attestation-fail-closed
Jun 22, 2026
Merged

Fail closed when gh is absent during attestation#21
TeoSlayer merged 1 commit into
mainfrom
fix/attestation-fail-closed

Conversation

@TeoSlayer

Copy link
Copy Markdown
Contributor

The bug (H5)

The auto-updater verifies SLSA provenance of checksums.txt via gh attestation verify, but verifyChecksumsAttestationFn did exec.LookPath("gh") and returned nil (PASS) whenever gh was absent. On headless production hosts the install.sh never installs gh, so the SLSA integrity gate was a silent no-op in production — collapsing auto-update integrity back to "anyone with GitHub repo-write access (compromised PAT, supply-chain compromise) can push a malicious release and every Pilot node auto-installs it."

A test (TestVerifyChecksumsAttestation_GhNotInstalled) even codified the skip-when-absent behavior, locking the vulnerability in.

The fix

  • Fail closed. When gh is not on PATH and SkipAttestation is false, verification now returns an error (gh CLI required for SLSA attestation verification; install gh or set SkipAttestation) and the update is refused. There is no implicit skip.
  • Explicit opt-out only. The existing Config.SkipAttestation (default false) is now the sole bypass — intended for test repos that lack real attestations. Its doc comment is updated to state the security implication.
  • SHA256 stays mandatory. The checksums.txt SHA256 match is unchanged and enforced regardless of SkipAttestation.
  • No bypass via either path. RunOnce (manual one-shot) and the gated checkLoop both funnel through applyUpdate, so neither can skip the gate.

Refactored the production check into a named realVerifyChecksumsAttestationFn so tests can restore it (the suite's TestMain stubs the package var to a nil-returning func).

Tests

  • Replaced the "gh absent => pass" test with one asserting fail closed.
  • Added: gh-absent + SkipAttestation=false via the wrapper => error.
  • Added: SkipAttestation=true end-to-end still requires a SHA256 match (installs on match, refuses on mismatch) — with the real attestation fn restored, proving SkipAttestation is the live bypass, not the test stub.
  • Existing happy path still passes.

GOWORK=off go build ./... && go vet ./... && go test -race ./... all green; edited files gofmt-clean.

Follow-up (web4)

The standalone cmd/updater lives in web4, not this module. A --skip-attestation flag / env should be threaded there to surface Config.SkipAttestation, mirroring the existing --state-path flag. No cmd/ exists in this repo, so nothing to thread here.

The SLSA provenance gate for checksums.txt called gh attestation verify,
but verifyChecksumsAttestationFn returned nil (pass) whenever gh was not
on PATH. Headless production hosts never install gh (install.sh omits it),
so the provenance check was a silent no-op there — collapsing auto-update
integrity back to "anyone with GitHub repo-write access can ship a
malicious release."

Make attestation fail closed: when gh is missing and SkipAttestation is
false, verification now returns an error and the update is refused. The
only bypass is the explicit Config.SkipAttestation opt-out (intended for
test repos without real attestations). The mandatory SHA256 checksums.txt
match is unchanged and still enforced regardless of SkipAttestation, and
both RunOnce and the gated checkLoop share applyUpdate so neither can
skip the gate.

Replace the test that codified "gh absent => pass" with one asserting
fail-closed, and add coverage for SkipAttestation=true still requiring a
SHA256 match (pass on match, refuse on mismatch).
@codecov

codecov Bot commented Jun 22, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@TeoSlayer TeoSlayer merged commit 372fda4 into main Jun 22, 2026
3 checks passed
@matthew-pilot matthew-pilot deleted the fix/attestation-fail-closed branch June 22, 2026 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants